-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace table
with va-table
web component
#1817
Conversation
…le.drupal.liquid files
Signed-off-by: Micah Chiang <[email protected]>
{% if forloop.first == true %} | ||
<va-table-row slot="headers" key="header"> | ||
{% for column in value %} | ||
{% if column == "" or column == nil %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how, but ---
was being rendered to fields that came through as empty or null, so I've added a check here and just render a span with some whitespace if the value is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a default behavior of the web component for handling missing data: https://design.va.gov/storybook/?path=/docs/components-va-table--missing-data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint is disabled
vets-website
uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.
What you can do
See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.
table
with va-table
web component
The code changes LGTM, just want to confirm that the visual changes are expected / desired. No more borders around non-table-header cells, and the text-alignment feels different The lack of borders appears consistent with: https://design.va.gov/storybook/?path=/docs/components-va-table--missing-data but I'm not as sure about the alignment? |
@tjheffner Thanks for the review! Yes, the alignment is expected with the web component. We have an outstanding issue in our backlog to clarify this in our documentation: department-of-veterans-affairs/vets-design-system-documentation#1871 |
Summary
<table>
with<va-table>
in thetable.drupal.liquid
file.Tested manually using the page found at
/housing-assistance/home-loans/funding-fee-and-closing-costs/
Some things to note:
Related issue(s)
closes department-of-veterans-affairs/vets-design-system-documentation#2217
Testing done
Screenshots
Desktop screen changes
before:after:
Mobile screen changes
before:after:
What areas of the site does it impact?
(Describe what parts of the site are impacted if code touched other areas)
Acceptance criteria
<table>
are replaced properly with<va-table>
and<va-table-row>
Quality Assurance & Testing
Error Handling
Authentication
#sitewide-public-websites
Slack channel for questionsRequested Feedback
(OPTIONAL) What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?